-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow LinePlot to be drawn in steps #99
base: master
Are you sure you want to change the base?
Conversation
Thanks @larshei - it looks great! It is a breaking change to the API so what I might do is add back the Edit: Just saw the last commit 🙄 |
lib/chart/lineplot.ex
Outdated
# keep backwards compatibility with old `:smoothed` option | ||
style = get_option(plot, :plot_style) |> IO.inspect(label: "style") | ||
smoothed = get_option(plot, :smoothed) |> IO.inspect(label: "smoothed") | ||
plot_style = if smoothed != nil, do: smoothed, else: style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be:
plot_style = if smoothed != nil, do: :smooth, else: style
(i.e. :smoothed
atom set)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - I see what you've done - the true/false is handled in the path logic. I think it's better to definitively set the plot_style here for a couple of reasons:
- We isolate the deprecation logic to a single spot
- The paths code may be used from elsewhere in the future and the options (true, false, :smooth, :direct, :step) are overlapping and unclear
- There are fewer places to go to remove the deprecated code
so something like:
plot_style = case smooth do
true -> if smoothed, do: :smooth, else: :direct
_ -> style
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted to
plot_style =
case smoothed do
true -> :smooth
false -> :direct
_ -> style
end
lib/chart/svg.ex
Outdated
@@ -85,7 +85,11 @@ defmodule Contex.SVG do | |||
|
|||
defp path([], _), do: "" | |||
|
|||
defp path(points, false) do | |||
defp path(points, nil), do: path(points, :smooth) | |||
defp path(points, true), do: path(points, IO.inspect(:smooth)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to remove the calls to IO.inspect()
once we're happy with the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the whole distinction on the function level and moved the mapping to the calling function, as you suggested above.
Changes
Many timeseries depict a "state" of some sort that was measured at specific points in time.
Simply drawing a line between the points of measurement often feels weird to me. I rather think "the value has been y from t1 to t2, then at t2 we got a new value".
So I added a style for line graphs that creates a LinePlot with steps:
As we no longer have two options (smooth yes/no) but three options,
the interface needed to be changed:
The option is now called
plot_type
, with the options:direct
:smooth
:step
If
plot_type
is not provided, we fall back to thesmoothed
option.